fix: propagate session permissions correctly to sub-sessions by Piyush0049 · Pull Request #3542 · docker/docker-agent

Piyush0049 · 2026-07-08T17:35:02Z

What this PR does / why we need it:
This PR resolves an open TODO regarding permission scoping during agent delegation.

Previously, when a parent session delegated a task to a sub-session (such as a background agent), the parent's explicit PermissionsConfig (Allow/Deny/Ask rules) was not inherited by the child session. Because background tasks often run with ToolsApproved: true, this created a potential gate-bypass where a sub-agent could execute tools that the user explicitly denied in the parent session.

Now that the runtime fully supports per-session permission scoping, this PR updates newSubSession to correctly inherit and append the parent's permissions.

Which issue(s) this PR fixes:
Fixes an inline TODO in pkg/runtime/agent_delegation.go.

Special notes for your reviewer:

Added TestSubSessionInheritsPermissions to pkg/runtime/agent_delegation_test.go to explicitly verify that the Allow, Ask, and Deny rules are correctly propagated to child agents.

Sub-agents now inherit the exact Allow, Ask, and Deny rules from the parent session instead of bypassing them. This resolves a known prompt-injection loophole for background agents. Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 · 2026-07-08T17:38:00Z

Note: Learning from previous PRs, I also added a unit test to verify that this behavior works correctly and stays fixed in the future. You can run the test locally using this command:
go test -v -run TestSubSessionInheritsPermissions ./pkg/runtime/...

Sayt-0

Thanks for tackling this. Inheriting the parent permissions in newSubSession is the right insertion point. Two points look blocking before this can land, plus a test gap; details are inline.

area	concern
correctness	background agents run with ToolsApproved: true, and Decide short-circuits on the yolo flag before any checker runs, so the inherited Allow/Ask/Deny rules are never consulted on that path (the exact case the removed TODO described)
isolation	the parent PermissionsConfig pointer is shared with the child instead of cloned, unlike every other call site
tests	the new test asserts field copying only, not dispatch behaviour

Sayt-0 · 2026-07-09T09:30:22Z

 		opts = append(opts, session.WithAgentName(cfg.AgentName))
 	}
+	if parent.Permissions != nil {
+		opts = append(opts, session.WithPermissions(parent.Permissions))


This shares the parent's *PermissionsConfig (and its underlying slices) with the child instead of cloning it. Every other call site clones through clonePermissionsConfig (session.Clone, copySessionMetadata in branch.go, store.go). The aliasing is not benign: on the interactive transfer_task path, handleResume's ResumeTypeApproveTool case appends to sess.Permissions.Allow, so a child approving a tool would mutate the parent's permissions (and append to a shared slice). Suggest cloning here, for example by exposing the existing clonePermissionsConfig helper and passing session.WithPermissions(clone).

Sayt-0 · 2026-07-09T09:30:22Z

@@ -482,10 +485,6 @@ func (r *LocalRuntime) CurrentAgentSubAgentNames() []string {
 // authorises all tool calls made by the sub-agent when they approve
 // run_background_agent. Callers should be aware that prompt injection in
 // the sub-agent's context could exploit this gate-bypass.


Removing this TODO looks premature. RunAgent (background agents) sets ToolsApproved: true, and Decide (pkg/runtime/toolexec/permissions.go) returns Allow on the yolo flag before evaluating any checker. So the newly inherited parent Deny/Ask rules are never consulted for a background sub-session, which is precisely the bypass this TODO called out ("rather than a single shared ToolsApproved flag"). The warning just above ("could exploit this gate-bypass") still holds, so it now contradicts the TODO removal. Options: keep the TODO, or make Deny/ForceAsk win over the yolo flag in Decide (at least for an inherited session-level Deny) and cover it with a test.

Sayt-0 · 2026-07-09T09:30:22Z

+	require.NotNil(t, s.Permissions)
+	assert.Equal(t, perms.Allow, s.Permissions.Allow)
+	assert.Equal(t, perms.Deny, s.Permissions.Deny)
+	assert.Equal(t, perms.Ask, s.Permissions.Ask)


This checks field copying but not that inheritance changes dispatch behaviour. Consider driving the approval path instead: a parent Deny of write_* should still deny in the child, and in particular the background-agent case (ToolsApproved: true) where the inherited Deny currently has no effect. That case would document the gap flagged in agent_delegation.go.

- Deep clone parent permissions into the child session to prevent aliasing - Re-order yolo check in Decide to ensure inherited Deny/ForceAsk overrides ToolsApproved: true - Enhance TestSubSessionInheritsPermissions to assert actual dispatch behavior Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 · 2026-07-09T10:36:44Z

@Sayt-0 Thank you for the review, I read it and have pushed an update addressing all three of your points:

Isolation / Aliasing: Added a public .Clone() method to PermissionsConfig and updated newSubSession to use it, ensuring child permissions don't alias the parent. (Also cleaned up the old unexported helper in pkg/session).
Correctness (YOLO Flag): Re-ordered the logic in Decide() so that explicit Deny and ForceAsk rules now properly win over the ToolsApproved: true (yolo) flag for background agents.
Dispatch Test: Updated TestSubSessionInheritsPermissions to explicitly drive the approval path, proving that a background agent (with the yolo flag on) correctly returns OutcomeDeny when inheriting a parent's denial.

All tests are passing locally. Do let me know if any change is required please.

Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 · 2026-07-09T16:41:28Z

I just pushed a quick follow-up commit to fix the CI failures.

Sayt-0 · 2026-07-10T11:31:58Z

HI @Piyush0049 can you resolve the conflicts ? ping me when its done

Piyush0049 · 2026-07-10T11:38:53Z

Sayt-0 Sure

…sions # Conflicts: # pkg/runtime/agent_delegation.go # pkg/runtime/agent_delegation_test.go

…sions # Conflicts: # pkg/runtime/toolexec/dispatcher.go

Piyush0049 · 2026-07-10T11:57:58Z

Sayt-0 I have resolved the merge conflicts. Please do let me know if any changes are required.

Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 · 2026-07-10T13:57:13Z

I've pushed a commit to fix the CI timeout in build-and-test.

The TestForceAskOverridesYoloMode test was hanging because it routed to askUser(), which blocked indefinitely waiting for UI input. Adding sess.NonInteractive = true allows it to correctly fail fast without hanging.

Side Note: While testing locally on Windows, I noticed a few pre-existing issues in main that break the local test suite (e.g. syscall.Mkfifo breaks compilation, hardcoded /abs/foo.go paths fail filepath.IsAbs, and sh/jq commands in hooks). Since these pass on Linux CI, I kept them out of this PR, but let me know if you'd like a separate PR later to make the tests Windows compatible

Piyush0049 · 2026-07-10T14:24:35Z

The build-image CI job probably failed due to a transient apt-get mirror sync error on download.docker.com. Could a maintainer please re-run the failed jobs when they get a chance?

Piyush0049 · 2026-07-10T16:02:31Z

Thanks for re-running the jobs. Please do let me know if any other changes are required.

Sayt-0

Thanks for the follow-up and for iterating on the earlier feedback. The isolation fix (PermissionsConfig.Clone()) is clean, and the race-prone double-cloning issue from the earlier round looks closed (verified locally with go test -race ./pkg/runtime/... ./pkg/session/..., no race, all green, matching CI).

One design point needs a decision before this can merge, plus a few smaller items, all left as inline comments.

Area	Concern	Blocking
Scope / design	`Decide()` reordering makes `Deny` and `ForceAsk` win over `--yolo` globally, not just for inherited sub-sessions	yes, needs a decision
Docs consistency	Several comments and one public doc page still describe the old ordering	yes
Diff hygiene	A few hunks are pure reformatting unrelated to the fix	no, nit
Test depth	New test exercises `Decide()` directly rather than the full dispatch path	no, nit

What's good:

PermissionsConfig.Clone(): exported, nil-safe, doc-commented, consistent with Session.ClonePermissions().
TestSubSessionInheritsPermissions is a real improvement over asserting field copies only.
No new allocations or performance concerns; slices.Clone is equivalent to the helper it replaces.

Sayt-0 · 2026-07-10T16:07:16Z

 		}
 	}

+	if yoloApproved {


This reorders the approval pipeline for every session, not just sub-sessions with inherited permissions.

Deny winning over --yolo matches the contract PermissionsConfig has documented since it was introduced (pkg/config/latest/types.go, and every frozen version v3 to v11): "Deny: Tools matching these patterns are always rejected, even with --yolo".

ForceAsk winning over --yolo reverses commit 7351e4fa0 ("--yolo must accept any tool call"), which added TestYoloMode_OverridesForceAsk (renamed here to TestForceAskOverridesYoloMode, assertion flipped) specifically to lock in the opposite behavior, and documented it in pkg/permissions/permissions.go's CheckWithArgs comment: "Note that --yolo mode takes precedence over ForceAsk" (still present on main, now stale).

The doc comment on this function, line 61, still lists yolo as step 1 ahead of the checkers; it needs updating either way this lands.

Question: should this only reorder the Deny case and leave ForceAsk behind --yolo (matching the two contracts above), or is overriding ForceAsk too an intentional posture change? If the latter, pkg/permissions/permissions.go and docs/tools/background-agents/index.md:33 ("via YOLO mode or explicit allow rules") need updating in this PR.

It was not an intentional posture change! I have updated the logic to only reorder the Deny case, leaving ForceAsk behind --yolo as originally intended.

Because this preserves the original contract, the external docs (pkg/permissions/permissions.go and index.md) remain accurate and didn't need updating. (I've also fixed the doc comment on Decide() to reflect this correctly).

Sayt-0 · 2026-07-10T16:07:16Z

 	}, "shell", nil, false)

-	assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d)
+	assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "team"}, d)


Matches the documented PermissionsConfig contract for Deny, no objection to this one specifically. See the comment on pkg/runtime/toolexec/permissions.go about the related ForceAsk case that still needs a decision.

Sayt-0 · 2026-07-10T16:07:16Z


-func TestYoloMode_OverridesForceAsk(t *testing.T) {
+// TestForceAskOverridesYoloMode verifies that ForceAsk permissions take precedence over the yolo flag.
+func TestForceAskOverridesYoloMode(t *testing.T) {


This test (renamed from TestYoloMode_OverridesForceAsk) now expects the tool NOT to execute, same for TestSessionDenyOverridesYoloMode below. This is the ForceAsk-over---yolo change flagged on pkg/runtime/toolexec/permissions.go, holding these pending that decision.

Separately, sess.NonInteractive = true on line 2602 fixing the CI hang (askUser blocking on a resume channel with no listener) is correct and worth keeping regardless of the outcome above.

Sayt-0 · 2026-07-10T16:07:16Z

 	}
 	if genai.EmitLegacyAttributes() {
-		delegationAttrs = append(delegationAttrs,
+		delegationAttrs = append(


This hunk (and the applyForceHandoff closing-paren move further down) is pure reformatting unrelated to the fix. Confirmed with golangci-lint fmt that neither the old nor the new layout is enforced by the formatter, so this is incidental noise. Suggest reverting both to keep the diff focused.

Sayt-0 · 2026-07-10T16:07:16Z


 	parent := session.New(session.WithUserMessage("hello"))
-	childAgent := agent.New("worker", "a worker agent",
+	childAgent := agent.New(


Same as the note on agent_delegation.go: the agent.New(...) reflow here (and 3 more occurrences: lines 194, 414, 471) is unrelated to the fix and adds review noise. Suggest reverting.

Sayt-0 · 2026-07-10T16:07:16Z

+		{Checker: checker, Source: "session permissions"},
+	}
+
+	decision := toolexec.Decide(s.ToolsApproved, namedCheckers, "write_file", map[string]any{"path": "foo"}, false)


Nit, not blocking: this builds a permissions.Checker by hand from s.Permissions and calls toolexec.Decide directly. That's a real improvement over asserting field copies only, but it still bypasses the actual wiring in permissionCheckers / rt.processToolCalls. Consider complementing it with an end-to-end case through RunAgent/runCollecting with a real background sub-session, similar to TestDenyOverridesYoloMode, so the real dispatcher path is covered too.

Restores YOLO overriding ForceAsk while keeping Deny overriding YOLO. Reverts unrelated formatting noise in agent_delegation.go and tests. Adds end-to-end TestRunAgent_EndToEndPermissions to verify dispatch path inheritance.

Piyush0049 · 2026-07-10T17:00:10Z

Thanks for the review. I read it and have addressed all the points in the table:

Scope/design: Decided to revert the ForceAsk change. It now stays behind --yolo as originally intended, while Deny correctly continues to override it.
Docs consistency: Updated the Decide() block comment. Since the original contract for ForceAsk was restored, the external docs are still accurate and were left untouched.
Diff hygiene: Reverted the formatting noise in agent_delegation.go (both the EmitLegacyAttributes hunk and the applyForceHandoff closing paren) and the agent.New() reflows in the test file.
Test depth: Fixed the expectation on TestYoloMode_OverridesForceAsk (kept the CI hang fix) and added a brand new TestRunAgent_EndToEndPermissions to verify the permissions.Checker end-to-end through the real RunAgent dispatcher path.

Please do let me know if any other changes are required.

fix: propagate session permissions correctly

a7c749b

Sub-agents now inherit the exact Allow, Ask, and Deny rules from the parent session instead of bypassing them. This resolves a known prompt-injection loophole for background agents. Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 requested a review from a team as a code owner July 8, 2026 17:35

aheritier added kind/fix PR fixes a bug (maps to fix:). Use on PRs only. area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection labels Jul 8, 2026

Sayt-0 reviewed Jul 9, 2026

View reviewed changes

test: update stale yolo tests and restore branch cloning

425b99a

Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

Piyush0049 added 2 commits July 10, 2026 17:21

Merge remote-tracking branch 'origin/main' into fix/subsession-permis…

e21bc2b

…sions # Conflicts: # pkg/runtime/agent_delegation.go # pkg/runtime/agent_delegation_test.go

Merge remote-tracking branch 'origin/main' into fix/subsession-permis…

7dbcbcb

…sions # Conflicts: # pkg/runtime/toolexec/dispatcher.go

Piyush0049 added 2 commits July 10, 2026 17:36

chore: fix formatting for gofumpt and gci

96d089e

Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>

test(runtime): fix TestForceAskOverridesYoloMode hanging on CI

25a6f4a

Sayt-0 requested changes Jul 10, 2026

View reviewed changes

fix: address maintainer feedback on permission overrides

38c27a8

Restores YOLO overriding ForceAsk while keeping Deny overriding YOLO. Reverts unrelated formatting noise in agent_delegation.go and tests. Adds end-to-end TestRunAgent_EndToEndPermissions to verify dispatch path inheritance.

Uh oh!

Conversation

Piyush0049 commented Jul 8, 2026

Uh oh!

Piyush0049 commented Jul 8, 2026

Uh oh!

Sayt-0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piyush0049 commented Jul 9, 2026

Uh oh!

Piyush0049 commented Jul 9, 2026

Uh oh!

Sayt-0 commented Jul 10, 2026

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Sayt-0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piyush0049 commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sayt-0 left a comment •

edited

Loading